Skip to content

fix: descend all legs for requireTimeCondition (#17407) - #20441

Open
NishthaShah wants to merge 15 commits into
apache:masterfrom
NishthaShah:fix/17407-require-time-condition-join
Open

NishthaShah wants to merge 15 commits into
apache:masterfrom
NishthaShah:fix/17407-require-time-condition-join

Conversation

@NishthaShah

@NishthaShah NishthaShah commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #17407.

Description

druid.sql.planner.requireTimeCondition has two defects in opposite directions, both stemming from how NativeQueryMaker#runQuery resolves "the intervals that apply to this query":

A minimal repro rejected on master today, although its only table access is bounded:

SELECT COUNT(*) FROM (
  SELECT dim1 FROM druid.foo WHERE __time >= '2000-01-01' LIMIT 10
)

Fix: resolve requireTimeCondition outside ExecutionVertex

ExecutionVertex#getEffectiveQuerySegmentSpec is load-bearing for execution — it feeds segment selection in CachingClusteredClient, walker lookup in BaseQuery, and interval handling in Queries and BrokerQueryResource. Changing it would change which segments get scanned. requireTimeCondition is asking a different, purely-validation question ("is there a __time filter anywhere that bounds the base scan?") which is not bounded by the execution vertex.

A new RequireTimeConditionAnalyzer (package org.apache.druid.query.planning) walks the query tree via ExecutionVertexShuttle and returns true iff every physical leaf datasource is descended from an ancestor query with a bounded QuerySegmentSpec (i.e. not Intervals.ONLY_ETERNITY). Global datasources (lookups, inline) short-circuit as satisfied.

Key invariants preserved by the walker:

  1. Descends into every QueryDataSource with no mayCollapseQueryDataSource gate — covers non-collapsible subquery stacks.
  2. Descends the left input of every JoinDataSource but not the right — a __time filter on the right side does not bound the base table. Preserves testRequireTimeConditionSemiJoinNegative.
  3. Keys on query intervals, not filters — a __time predicate over a subquery result stays a residual filter and never becomes a QuerySegmentSpec, so it must not satisfy the guardrail (testRequireTimeConditionOuterFilterOverUnboundedSubqueryNegative).

Scope

NativeQueryMaker is the only enforcement site touched. MSQTaskQueryMaker, DartQueryMaker, and PrePlannedDartQueryMaker do not currently check requireTimeCondition at all, so the flag is silently a no-op on those paths. Extending the check to those makers is out of scope for this PR.

Release note

Fixed a bug in druid.sql.planner.requireTimeCondition where queries with a __time filter on every physical leg were rejected as if they had no filter. Affected shapes include joins of two bounded subqueries (pre-33 regression) and non-collapsible subquery stacks over bounded scans (33+ regression).


Key changed/added classes in this PR
  • RequireTimeConditionAnalyzer (new)
  • NativeQueryMaker
  • CalciteQueryTest

This PR has:

  • been self-reviewed.
  • added unit tests covering new code paths (seven new tests in CalciteQueryTest, spanning explicit joins, three-way joins, lookups on the right side of a join, non-collapsible subqueries, and outer filters over unbounded subqueries; all four pre-existing positive and three pre-existing negative requireTimeCondition tests continue to pass).
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.

The requireTimeCondition guard has two defects in opposite directions:

* Pre-33: joins of two bounded subqueries were rejected because
  DataSourceAnalysis.getBaseQuerySegmentSpec resolved the outer join
  query first, whose interval is ETERNITY.

* v33+: non-collapsible subquery stacks are rejected because
  ExecutionVertex.getEffectiveQuerySegmentSpec prunes at
  Query.mayCollapseQueryDataSource (default false), so the inner
  query is never visited.

Resolve the check outside ExecutionVertex with a walker that descends
every QueryDataSource and the left input of every JoinDataSource. The
right-hand input of a join is intentionally not followed (preserving
testRequireTimeConditionSemiJoinNegative), and the guard keys on query
intervals rather than filters (preserving rejection of outer __time
filters over unbounded subqueries).

Adds nine tests covering explicit joins, three-way joins, UNION ALL,
lookups on the right side of a join, non-collapsible subqueries, and
outer filters over unbounded subqueries.
@NishthaShah
NishthaShah marked this pull request as draft September 28, 2026 07:39
…imitations)

Both testRequireTimeConditionUnionAllBothBranchesFilteredPositive and
testRequireTimeConditionUnionAllOneBranchMissingFilterNegative fail
under MSQ and Decoupled planners for reasons unrelated to the
requireTimeCondition fix:

* MSQ rejects UNION ALL between filtered scans at planning time
  ('SQL requires union between inputs that are not simple table
  scans') before NativeQueryMaker's guard runs.
* Decoupled produces a top-level UnionQuery (not a BaseQuery); the
  ~20 ExecutionVertex.of(query) callsites upstream of the analyzer
  throw 'Can't traverse a query[UnionQuery]!'.

The base-planner UNION ALL cases pass and the analyzer handles the
tree correctly — supporting UNION ALL under MSQ/Decoupled would
require orthogonal fixes to ExecutionVertexShuttle and MSQ's union
planner, out of scope for this PR.

The remaining 14 requireTimeCondition tests still cover both regression
shapes from the RCA: non-collapsible subquery stacks (v33+) and joins
of two bounded subqueries (pre-33).
…rage

Seven direct-call tests exercise the analyzer's paths so the
diff-coverage check passes in the processing module (the sql-module
CalciteQueryTest coverage is not counted toward this file). Covers
bounded/unbounded scans, nested QueryDataSource descent (v33+
regression), global datasource short-circuit (LookupDataSource,
InlineDataSource), and JoinDataSource leg-boundary detection.

Coverage on RequireTimeConditionAnalyzer$TimeFilterExplorer:
98% instruction, 85% branch (was 0% / 0% from processing tests).
@NishthaShah
NishthaShah marked this pull request as ready for review October 5, 2026 09:59

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fix interval attribution before merging. The analyzer currently allows a bounded outer query to satisfy the guard for an unbounded nested physical scan, so requireTimeCondition can still permit a full table scan in a subquery stack.

Reviewed 4 of 4 changed files.

Validation: mvn -ntp -pl processing -Dtest=org.apache.druid.query.planning.RequireTimeConditionAnalyzerTest -Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks -Dweb.console.skip=true -T1C test passed (7 tests); git diff --check edea417869a2f2fcacd3d1f81b7ef3776908092e d9bea4c4028722ac6ef97fc9a4373b27cf5981d7 passed.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

crossedJoinLegBoundary = true;
}
}
if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not let an outer interval mask an unbounded inner scan

Finding: The loop keeps searching past an unbounded query and returns as soon as any outer ancestor is bounded. For a non-collapsible stack such as a bounded outer query over an ETERNITY inner query, the physical leaf is executed by the inner query with no time interval, but hasTimeFilterOnAllLegs returns true and NativeQueryMaker allows it. This lets requireTimeCondition permit the full scan it is meant to reject.

Suggestion: Evaluate the interval at the innermost query that owns the leaf, while still respecting join-leg boundaries, and add a regression test for a bounded outer query over an unbounded inner query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Pushed a regression commit that adds two tests in RequireTimeConditionAnalyzerTest to cover the "bounded outer over unbounded inner" shape being called out:

Verified the fix end-to-end for the specific SQL query for (outer interval mask an unbounded inner scan)

SELECT COUNT(*) FROM wikipedia foo JOIN (SELECT page, __time FROM wikipedia LIMIT 10) bar ON foo.page = bar.page WHERE foo.__time >= TIMESTAMP '2015-01-01';

Native plan is Timeseries([2015-01-01/∞)) over Join(TableDataSource(wikipedia), QueryDataSource(ScanQuery(wikipedia, ETERNITY, LIMIT 10))) — the exact P1 shape.
- Pre-fix (ExecutionVertex.getEffectiveQuerySegmentSpec only): wrongly succeeds, returns 17. The outer [2015-01-01/∞) masks the unbounded inner scan.
- Post-fix (walker descends every join leg): correctly rejects with CannotBuildQueryException("requireTimeCondition is enabled, all queries must include a filter condition on the __time column").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up assessment

Reviewed 4 of 4 changed files. The new tests cover the reported right-join leg and nested right-leg cases, including the SQL shape in your reply. One related case still reaches a bounded ancestor: a non-collapsible QueryDataSource over an ETERNITY inner query outside a join-right leg. Here crossedJoinLegBoundary remains false, so line 109 returns true for the bounded outer query even though the inner query owns the unbounded physical scan. Please add a regression test for this shape and stop the ancestor search at the inner execution boundary.

Validation: Static review; git diff --check origin/master...HEAD passed. Tests were not run.

…nAnalyzer

Add two regression tests for the Codex P1 raised on apache#20441:

* testJoinWithLimitWrappedUnboundedRightIsNotSatisfied — a LIMIT-wrapped
  QueryDataSource on the right leg of a join under a bounded outer scan;
  LIMIT blocks __time predicate push-down, so the inner leaf stays at
  ETERNITY. The walker must cross the join-leg boundary before reaching
  the bounded ancestor and reject.

* testNestedJoinWithDeepUnboundedLegIsNotSatisfied — a nested
  Join(FOO, Join(FOO, BAR)) under a bounded outer scan; the deepest leg
  crosses two join-leg boundaries, exercising transitive descent.

Both guard against the pre-fix behavior where
ExecutionVertex.getEffectiveQuerySegmentSpec would return the outer
interval and silently accept the unbounded inner scan.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The added tests cover the unbounded right join leg from the reported SQL shape, including nested join traversal. A separate non-collapsible QueryDataSource path remains: when the inner query has ETERNITY and an outer query is bounded without crossing a join-right boundary, crossedJoinLegBoundary stays false and this loop accepts the outer interval. The inner query still owns the physical scan, so requireTimeCondition can allow an unbounded scan.

Reviewed all 4 changed files: RequireTimeConditionAnalyzer.java, RequireTimeConditionAnalyzerTest.java, NativeQueryMaker.java, and CalciteQueryTest.java.

Validation: git diff --check origin/master...HEAD passed. Static review only; tests were not run.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

crossedJoinLegBoundary = true;
}
}
if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not let a bounded outer query mask an unbounded inner scan

Finding: When a non-collapsible QueryDataSource wraps an unbounded inner query outside a join-right leg, this loop continues past the inner query and returns true for a bounded outer query. For example, a bounded query over QueryDataSource(scan(table, ETERNITY)) leaves the physical table scan owned by the unbounded inner query, yet requireTimeCondition is accepted and permits the full scan.

Suggestion: Treat a non-collapsible QueryDataSource as an execution boundary and require the query owning each physical leaf to have a bounded segment spec; add a regression test for a bounded outer query over an ETERNITY inner query.

Addresses Frank's follow-up P1 on apache#20441. The walker was returning true
for a bounded outer query over an unbounded inner QueryDataSource outside
any join-right leg, because the ancestor loop climbed past the inner
query (unbounded) and accepted the outer's bound as sufficient. The
physical table scan is actually owned by the inner query, so the outer's
bound does not reach the leaf unless the chain between leaf and outer
is fully collapsible.

Track collapsedChain while walking ancestors: for every Query past the
innermost, &= q.mayCollapseQueryDataSource(). Only accept a bounded
Query ancestor when the chain between the leaf and that ancestor has
collapsed. The innermost Query owns the leaf directly and does not need
a collapsibility check. Preserves existing behavior for
GroupBy-over-GroupBy (where mayCollapseQueryDataSource() is true) and
rejects Scan-over-Scan via QueryDataSource (where it is false).

Covered by new native test
RequireTimeConditionAnalyzerTest.testBoundedOuterOverUnboundedQueryDataSourceIsNotSatisfied.
Existing CalciteQueryTest#testRequireTimeCondition* cases unchanged.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The scan-over-scan bypass from the previous review is fixed, but a bounded outer groupBy still satisfies the guard for an inner groupBy that scans over ETERNITY. Address the interval attribution for that collapsible query chain before merging.

Reviewed 4 of 4 changed files. Started with both incremental files, then reviewed the full current diff, native-query integration, existing and added tests, execution-vertex interval selection, broker segment selection, and nested groupBy execution.

Validation: full and incremental git diff --check passed at head 9e4d0e20945f101de79ab26eea019bdf6abd5ae8. Static review only; no tests or builds were run.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

// absorbed into q (or further up). Each hop needs the outer Query to
// mayCollapseQueryDataSource() its inner QueryDataSource; otherwise the
// inner query runs independently and the outer's bound does not reach the leaf.
collapsedChain &= q.mayCollapseQueryDataSource();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not treat groupBy collapsibility as interval pushdown

Finding: The new condition still accepts a bounded outer GroupByQuery over an unbounded inner GroupByQuery: GroupByQuery.mayCollapseQueryDataSource() returns true for this pair, so collapsedChain stays true and the outer interval satisfies the guard. That flag does not mean the outer interval is applied to the physical scan. ExecutionVertexExplorer records the innermost query's segment spec, CachingClusteredClient uses that spec for segment selection, and GroupByQueryQueryToolChest.mergeGroupByResultsWithoutPushDown executes the inner query with its original intervals before processing its results through the outer query. For example, replacing both scans in the new regression test with groupBy(TABLE_FOO, ETERNITY) and groupBy(new QueryDataSource(inner), BOUNDED) still returns true while the table is scanned over ETERNITY. This leaves requireTimeCondition bypassable for nested groupBy queries, which the pre-PR guard rejected using the innermost segment spec.

Suggestion: Require a bound on the query that actually owns each physical scan, and add the equivalent groupBy-over-groupBy regression test rather than assuming mayCollapseQueryDataSource propagates intervals.

mayCollapseQueryDataSource() is a datasource-collapse signal, not an
interval-propagation one. GroupByQuery returns true for it over a
QueryDataSource(GroupByQuery), but at runtime the inner query is executed
with its original intervals (see
GroupByQueryQueryToolChest#mergeGroupByResultsWithoutPushDown) and an
outer bound never reaches the physical scan.

Replace the ancestor-chain walk with the innermost-owning-query rule:
for each physical leaf, walk up to the first Query ancestor and check
its bound. Stop at join right-leg boundaries since the outer's spec
only applies to the primary (left) input.

Adds adversarial coverage for the follow-up P1 raised on apache#20441
(bounded outer GroupBy over ETERNITY inner GroupBy) plus five scenarios
locking down over-rejection and under-rejection edges: right leg with
its own bounded Query, 3-layer QDS with only middle bounded, bounded
inner under ETERNITY outer, and UnionDataSource with bounded/unbounded
outer.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No actionable issues found in this review. The innermost-query interval check fixes the previously reported bounded-outer/unbounded-inner bypass, including nested groupBy, while preserving bounded inner scans and independent bounded right join legs. The earlier interval-attribution findings are resolved in the reviewed head.

Reviewed 4 of 4 changed files. Started with the 2 incremental files, then inspected the full current diff and surrounding query traversal, subquery materialization, nested groupBy execution, native-query enforcement, and SQL test verification.

Validation: full and incremental git diff --check passed at head 459eb6890641be69b0448db507054ea1d7357281. Static review only; no tests or builds were run.


This is an automated review by Codex GPT-5.6-Luna(max)

…n error

Expose a Result record from RequireTimeConditionAnalyzer carrying the first
offending physical leaf's datasource name and its structural position
(TOP_LEVEL, NESTED_SUBQUERY, JOIN_RIGHT). NativeQueryMaker composes a
position-specific message so the user sees which table and roughly where
the __time filter is missing, instead of a single generic string.
…onAnalyzer

Add four tests exercising the walker at 4-5 layers deep:
- Two pure-subquery depth-4 cases (bound at innermost / bound only in middle)
  confirm that only the innermost owning query's segment spec reaches the
  physical scan.
- Two mixed subquery-and-join cases confirm the same at depth with a join
  in the chain: three bounded subquery wrappers above a Join with a bare
  unbounded right leg reject at the right-leg boundary (JOIN_RIGHT); a
  five-layer mix with a bounded right-leg wrapper above an unbounded inner
  rejects at the inner query (NESTED_SUBQUERY).
…guard

Adds testRequireTimeConditionNestedRollupPositive to pin a common nested
GROUP BY shape at the SQL layer: an outer aggregate wrapping an inner
subquery whose GROUP BY owns the __time bound. The analyzer-level coverage
for this shape already exists via
testIntermediateBoundedInnerUnderUnboundedOuterIsSatisfied; this test adds
the end-to-end SQL assertion so the planner path stays regression-free.
@NishthaShah
NishthaShah force-pushed the fix/17407-require-time-condition-join branch from 93a992d to b296878 Compare October 12, 2026 08:52
Adds two tests exercising the CTE (WITH ... AS (...)) surface:

- testRequireTimeConditionCtePositive: a CTE that owns the __time bound in its
  definition inlines to a bounded inner subquery under the outer aggregate; the
  guard must accept it.
- testRequireTimeConditionCteNegative: a CTE with no __time filter anywhere
  inlines to an unbounded scan; the guard must reject with the "__time column"
  message.

Calcite inlines CTEs into equivalent nested subqueries (and unions when a CTE
is referenced more than once), so these shapes already map onto existing
analyzer coverage. These tests pin the SQL-layer behaviour so WITH syntax is
not an unaudited user-visible dimension.
…configs

Converts the nine testRequireTimeCondition*Positive* tests to JUnit 5
@ParameterizedTest, running each case under both PLANNER_CONFIG_REQUIRE_TIME_CONDITION
and PLANNER_CONFIG_DEFAULT. Each test executes 2 invocations (18 total) and
asserts that enabling the guard does not change the native plan or the result
rows for compliant queries.

Scope:
- testRequireTimeConditionPositive
- testRequireTimeConditionPositive2
- testRequireTimeConditionPositive3
- testRequireTimeConditionLogicalValuePositive
- testRequireTimeConditionExplicitJoinBothSidesFilteredPositive
- testRequireTimeConditionLookupRightSidePositive
- testRequireTimeConditionNonCollapsibleSubqueryPositive
- testRequireTimeConditionNestedRollupPositive
- testRequireTimeConditionCtePositive

Negatives remain plain @test with assertThrows: under require-off they would
plan as normal unbounded queries, which already has coverage elsewhere and
offers weak incremental signal.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No actionable issues found in this review. The current analyzer, native-query integration, diagnostic result handling, and planner-configuration test coverage are consistent with the intended requireTimeCondition behavior.

Reviewed 4 of 4 changed files. Started with the incremental diff, then inspected the full current diff and surrounding query/datasource traversal and SQL enforcement paths.

Validation: full and incremental git diff --check passed. Static review only; no tests or builds were run.


This is an automated review by Codex GPT-5.6-Luna(max)

…m recording

DecoupledPlanningCalciteQueryTest pins the native plan for this semi-join via
a frozen quidem .iq file keyed on the test's display name. Parametrising the
method made the harness look for per-config recordings
(testRequireTimeConditionPositive3@require-on.iq etc.) that don't exist,
breaking DecoupledPlanningCalciteQueryTest in CI.

Revert this one test to plain @test. The guard runs post-plan, so the matrix
adds no planner coverage here anyway. The other 8 positives stay parametrised.
Pins the end-to-end behaviour of the guard through the SQL planner + Avatica
path on two positive shapes (bounded nested-rollup, bounded CTE) and one
negative (top-level unbounded) that asserts the formatted error message.

Complements the existing CalciteQueryTest coverage: those tests exercise the
guard against the native query tree directly, while this one validates the
plan + results + error-message flow a JDBC client actually sees. Also pins the
mysql-formatted result tables, which protects against regressions in planning
that would otherwise silently change what the guard sees under it.
Boots an in-process cluster with druid.sql.planner.requireTimeCondition=true,
ingests a datasource via the standard BASIC_INDEX task, and submits SQL through
the Broker's HTTP endpoint. Verifies both the positive path (bounded query
returns results) and the negative path (unbounded query is rejected via a
400 Bad Request carrying the "has no filter on the __time column" message).

This complements the existing coverage:
  - RequireTimeConditionAnalyzerTest       -- analyzer semantics (unit)
  - CalciteQueryTest testRequireTimeCondition* -- planner-level SQL guard (unit)
  - requireTimeCondition.iq                -- quidem SQL regression recording
and closes the loop end-to-end through broker config wiring and the Avatica/HTTP
response path.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fix the global-datasource shortcut before merging. It currently exempts physical broadcast tables from requireTimeCondition, allowing direct or left-side unbounded scans to bypass the guard.

Reviewed 6 of 6 changed files.

Validation: full and incremental git diff --check passed. Static review only; no builds or tests were run.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

@Override
protected DataSource visit(DataSource dataSource, boolean leaf)
{
if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Global table leaves bypass the time guard

Finding: Defect: the blanket isGlobal() exemption treats GlobalTableDataSource as a non-physical global, although it extends TableDataSource and still represents segment-backed data. Trigger: a direct query or left join leg uses a broadcast datasource represented by GlobalTableDataSource with an ETERNITY interval. Impact: requireTimeCondition accepts the query and permits an unbounded scan of the broadcast table, defeating the configured guardrail.

Suggestion: Restrict the exemption to non-segment global datasources such as lookups and inline data, while preserving the intended right-side join behavior, and add direct/left-leg GlobalTableDataSource regression coverage.

… datasources

GlobalTableDataSource extends TableDataSource and remains segment-backed when
queried directly or on a join's left leg (see its javadoc). The blanket
dataSource.isGlobal() exemption at the leaf check therefore let an unbounded
scan of a broadcast datasource bypass requireTimeCondition.

Restrict the leaf-level exemption to leaves that are not instanceof
TableDataSource (lookups, inline, frame-based inline), and preserve the
right-leg broadcast-join path by short-circuiting isGlobal() leaves inside the
JOIN_RIGHT branch (where a GlobalTableDataSource is materialized via
JoinableFactory as an IndexedTable rather than scanned as segments).

Adds four analyzer-level regression tests covering direct (bounded + eternity),
left-leg under an unbounded outer, and right-leg broadcast-join cases for
GlobalTableDataSource.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No actionable issues found in this review. The current head fixes the GlobalTableDataSource exemption on direct and left-join scan paths while preserving valid global right-join behavior.

Reviewed 6 of 6 changed files.

Validation: full and incremental diff checks passed. Static review only; no builds or tests were run.


This is an automated review by Codex GPT-5.6-Luna(max)

@FrankChen021 FrankChen021 added the Approval Recommended Used when AI completes a review and reports no findings label Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approval Recommended Used when AI completes a review and reports no findings Area - Querying

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RequireTimeCondition does not handle complex joins

2 participants